Avoid raising bare Exception #1168
Conversation
facebook-github-bot
added
the
CLA Signed
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1168 +/- ##
==========================================
+ Coverage 91.26% 91.29% +0.03%
==========================================
Files 261 262 +1
Lines 26877 26899 +22
==========================================
+ Hits 24529 24558 +29
+ Misses 2348 2341 -7 ☔ View full report in Codecov by Sentry. |
|
This seems like an improvement, thank you. It's really hard to catch a specific |
| # Copyright (c) Meta Platforms, Inc. and affiliates. | ||
| # | ||
| # This source code is licensed under the MIT license found in the | ||
| # LICENSE file in the root directory of this source tree. | ||
|
|
||
|
|
||
| class CSTLogicError(Exception): | ||
| pass |
There was a problem hiding this comment.
This should go in _exceptions.py, and please write a docstring
There was a problem hiding this comment.
Thanks for the review !
I'm going to need help on this one though:
- It's in a separate
_excep.pyfile, since_exceptions.pyalready imports internals, which in turn may raise aCSTLogicError.
Do you see any other ways than my (I admit lazy) solution in order to avoid circular import errors ? - For the docstring, I couldn't think of anything meaningful, other than "Well... it's a CST Logic error", which doesn't add more info or context than the original name. I'd be glad to add a real doc if you have something better in mind :)
There was a problem hiding this comment.
@zsol
Any updates ?
There was a problem hiding this comment.
_exceptions.py already imports internals, which in turn may raise a CSTLogicError.
Let's move get_expected_str out of _exceptions.py. That should help.
For the docstring, I couldn't think of anything meaningful
How about:
General purpose internal error within LibCST itself.
… on exception messages)
|
Thanks for the support @kiri11 ! I hope everything is now ready (apart from that |
|
You're welcome! (although I didn't do anything) |
Summary
Before tackling issue #457, there are some bare Exceptions thrown from within the code.
Here is the logic for changes in Exceptions:
ValueErrorwhen issue caused by user inputCSTValidationError&TypeErrorwhen issue occurs during the CST validation processParserSyntaxErrorwhen issue occurs during the parsing processCSTLogicErrorto replace all Exception which contained "Logic error" in the cause messageI'm all for changes if some Exceptions changes are not OK,
In particular, there are some
ParserSyntaxErrorwhich may not be meaningful - with raw_line=0, raw_column=0 paramsTest Plan
Keep the current test plan.